Refactor: observability code cleanup#17862
Refactor: observability code cleanup#17862sufeng-buaa wants to merge 1 commit intosgl-project:mainfrom
Conversation
sufeng-buaa
requested review from
ByronHsu,
CatherineSue,
JustinTong0323,
ShangmingCai,
Ying1123,
ch-wan,
fzyzcjy,
hanming-lu,
hnyls2002,
ispobock,
merrymercy,
slin1237,
xiezhq-hermann and
yizhang2077
as code owners
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
bot
added
the
documentation
|
/tag-and-rerun-ci |
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
sufeng-buaa
force-pushed
the
sufeng-buaa/observability_integration
branch
from
fbb3f87 to
c381672
Compare
ShangmingCai
left a comment
ShangmingCai
left a comment
There was a problem hiding this comment.
The disaggregation part LGTM.
| global_diff_realtime_monotonic = time.time() - time.perf_counter() | ||
|
|
||
|
|
||
| def calibrate_time_diff(): |
There was a problem hiding this comment.
Using global_diff_realtime_monotonic is a good idea for converting perf_counter values to timestamps. However, when DP > 1, you may have a single tokenization manager but multiple schedulers running across different devices. In that case, you need to be careful: each process/device can have its own monotonic clock offset, so the conversion may be inconsistent across ranks unless those offsets are synchronized or computed per rank.
There was a problem hiding this comment.
Monotonic time is typically obtained via the Linux kernel's vDSO (or system call), and the Linux kernel ensures timestamp consistency across different CPUs. Even in extremely rare cases, non-observability functionalities only rely on time retrieved within the same process, while observability features can fully tolerate such minimal timing discrepancies.
There was a problem hiding this comment.
But if the tokenize manager and the scheduler are running on completely different machines, will the monotonic time they obtain still be consistent?
There was a problem hiding this comment.
Fixed. Correct monotonic time during deserialization by propagating it with a diff.
sufeng-buaa
force-pushed
the
sufeng-buaa/observability_integration
branch
2 times, most recently
from
2c1c70f to
8092b3f
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the observability code. The changes centralize timing, metrics, and tracing logic into new ReqTimeStats and TraceContext objects, which greatly improves code organization and maintainability. The new API for tracing is much cleaner and more powerful, with features like dynamic trace levels. The use of monotonic time and careful handling of time across processes are also commendable. Overall, this is a high-quality refactoring that enhances the observability of the system. My review includes a few minor suggestions for improving the documentation.
|
/rerun-failed-ci |
sufeng-buaa
force-pushed
the
sufeng-buaa/observability_integration
branch
2 times, most recently
from
c64a0eb to
457d9b6
Compare
|
The consolidation and tracing pieces overall LGTM. AI review https://app.devin.ai/review/sgl-project/sglang/pull/17862 concurs overall |
Signed-off-by: Feng Su <sufeng@linux.alibaba.com>
sufeng-buaa
force-pushed
the
sufeng-buaa/observability_integration
branch
from
457d9b6 to
5de694f
Compare
4 participants
Motivation
According to #17482, organize the observability-related code, remove redundant code, and unify the interfaces for time statistics, request latency metrics, and request tracing.
Modifications
Accuracy Tests
Benchmarking and Profiling
Checklist
Review Process
/tag-run-ci-label,/rerun-failed-ci,/tag-and-rerun-ci